Skip to content

PT-4575: Per-pane content zoom — chords, opt-in, tab menu, Settings, editor - #2821

Merged
rolfheij-sil merged 109 commits into
mainfrom
pt-4575-content-zoom
Sep 22, 2026
Merged

rolfheij-sil merged 109 commits into
mainfrom
pt-4575-content-zoom

Conversation

@rolfheij-sil

@rolfheij-sil rolfheij-sil commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #2803 (pt-4576-content-zoom-platform-core). Review only the commits above that branch. Stacked PRs get no automatic CI here; the full local battery (typecheck, lint, format:check, and the unit suites of root, platform-bible-react, platform-bible-utils and the Scripture editor extension) is green.
Battery on the combined branch: typecheck (core, erb, e2e, workspaces) 0 errors; lint 0 errors; format:check clean; vitest run src 270 files / 3994 passed / 1 skipped; Scripture editor extension 94 files / 1530 passed; platform-bible-react unit 70 files / 915 passed; platform-bible-utils 32 files / 611 passed; build:extensions and the platform-bible-react build (through typedoc) clean.
After #2803 squash-merges, this branch is rebased with git rebase --onto <new base> <old parent tip>.

Summary

This is the rest of the per-pane content-zoom epic (PT-4575), combining the five work items that followed the platform core in #2803. A user can now zoom one pane's content instead of scaling the whole app: Ctrl/⌘ + - 0 and Ctrl/⌘ plus the mouse wheel change the level of the pane under the pointer or caret, the Scripture editor zooms its text and its footnotes list independently, Zoom in / Zoom out / Reset zoom to default appear in the web-view tab menu and in the editor's own hamburger menu, Settings offers a percent stepper for the default level every pane starts at, and macOS gets an explicit View menu with the same three actions. Each pane remembers its level.

  • PT-4577, chord ownership and the macOS View menu. Main stops swallowing Ctrl + - 0 in before-input-event, so the bootstrap from PT-4576: Platform core for per-pane content zoom #2803 receives them. platform.zoomIn / platform.zoomOut stay as commands with no default chord, and their step goes through adjustZoomFactor, fixing the float drift that rejected the twentieth step. On macOS the View menu is explicit, with ⌘= / ⌘- / ⌘0 plus hidden numpad duplicates replacing the native zoom roles. A renderer top-document keydown listener covers window chrome, where no iframe sees the key. The keyboard-shortcuts catalog is updated.
  • PT-4580, the opt-in contract. ContentZoomRoot (platform-bible-react, @experimental) is a forwardRef div carrying data-platform-content-zoom-root; no area prop marks the view's main area, area="footnotes" names a second independently zoomable pane. CONTENT_ZOOM_ROOT_ATTRIBUTE is duplicated in the library on purpose and pinned equal to core's by a test. Author docs land in the Extension-Development-Guide and Component-Builder-Patterns, three ADR entries record the decisions, and the Paratext 9 inventory gains a "Pane Zoom (View > Zoom)" entry.
  • PT-4578, the tab menu. A platform.tabZoom group (order 50, not extensible, isExperimental) binds the three owner-routed commands from PT-4576: Platform core for per-pane content zoom #2803 ahead of the window group. Simple mode gets a tab menu for the first time, holding only that group; the tab title reads the contributed menu in both modes and narrows it before conversion. A tab hosting no web view gets no zoom items. This fulfils PT-3171.
  • PT-4579, the Settings default and layout. platform.webViewContentZoom renders as − · 100 % · + · reset through a new renderer-internal PercentStepper, writing through the existing validate → setSetting tail without the text box's debounce. "Zoom factor" becomes Interface scaling and "Zoom" becomes Tab content default zoom. General reads Interface language, Interface scaling, Tab content default zoom; a new Supporter settings group holds Request timeout and the re-registration reminder; Interface mode is hidden, because the profile popover carries that switch in both modes.
  • PT-4581, the Scripture editor. The Scripture text is the main area and the footnotes list the footnotes area, with the footnote-editing popover following the text's level; toolbar, divider and panel padding stay outside every area, so they keep their size. Editable and read-only editors share the web-view type and remember levels under editor:<projectId>:<area>. The editor's hamburger menu carries the three zoom items in both modes. Two platform fixes the first marked view exposed ship here: a click keeps its area active even when it moves focus elsewhere, and a pane whose replacement content runs no bootstrap drops its stale area list.

Why review this

This is the epic's user-visible half. It removes a long-standing app-wide shortcut on purpose, adds the first UI Simple mode offers on a tab, changes the tab title component every tab renders, and is the first proof of the opt-in contract from #2803.

Decisions worth a look

  • Chord ownership. The chrome listener accepts Shift only for the zoom-in keys, matching the bootstrap's rule; a parity test pins the two copies to each other. resolveContentZoomTarget returns nothing while a dialog is open and the chrome listener checks the same query, so dialogs stay out of scope on every input path, including the macOS accelerators. Enhanced Resources' private Ctrl chord handler becomes reachable again on Windows/Linux; removal is PT-4583.
  • ContentZoomRoot placement. The marker is rendered after the props spread, so area is always the source of truth. In the editor it goes inside PortalContents, because react-reverse-portal clones its direct child with the portal's props; editorContainerRef is untouched, since a marker there would nest the footnotes marker.
  • Simple mode reaches the tab menu only on Column 3. The Home and Scripture Editor columns use a headless tab bar, so the editor's own hamburger menu is what carries zoom in both modes.
  • Menu items cannot carry isExperimental. The item schema rejects unknown properties, so the group carries it, as platform.tabWindow already does.
  • Items stay inert on a view with no zoom area. The service treats such a request as a silent no-op, and the "has areas" signal fills asynchronously, so disabling on it would show disabled items that in fact work.
  • The relabel goes through a new key. "Zoom factor" is a shipped, immutable string, so Interface scaling arrives as %settings_platform_zoomFactor_label_2% with a deprecation entry, per the Localization Guide.
  • No optimistic readout in the stepper. The stored value round-trips through the extension host before the prop updates. The readout always shows the stored value; only the baseline for a rapid next press uses the last emitted factor, within a 1.5 s window and until the prop moves. Two earlier optimistic attempts each opened a stuck-or-flicker case.
  • The click-gesture rule. A focusin naming a different area within 200 ms of a pointerdown is ignored, so selecting a footnote row does not hand the active area back to the text; a focus change with no pointer behind it still sets the area.
  • Stale areas are dropped by bootstrap liveness, not by a "re-reported since this load" flag: the fresh bootstrap reports from DOMContentLoaded, before the iframe's load event, so clearing at load would race that report.
  • Keyboard-chord e2e cases are written but gated. Playwright drives keys through CDP, which never reaches before-input-event, so an ungated test would go green while the product path stayed untested. Ctrl+wheel and the commands cover the same paths.
  • Observer cost is below measurement. A throwaway probe under a typing burst saw zero or one mutation batch and a scan below performance.now() resolution, so nothing was filed and the probe was not kept.
  • Known follow-ups: the Simple-mode Home column has no menu of any kind and keeps only the chords (a UX question); the footnotes pane does not yet take the project font size (PT-4189); the indicator anchors to the union of main-marked elements, so the badge may sit at the footnote popover's corner while it is open; the platform-bible-react dist/ bundle is not byte-stable across builds, so a rebuild shows unrelated churn; a Storybook page for the epic is an open epic-level question; the macOS runtime check belongs to PT-4585.

Testing

  • Renderer and main: macOS menubar data (11), the chrome-key listener (19), the dialog-open query (3), keyboard-catalog location existence, overlay-store and dialog-shard cases, resolver gate cases, a float-drift regression, the percent stepper (19) with the setting component and the General/Supporter layout data, the tab-menu data, filterTabMenuToGroup, the tab title's zoom menu (11), and bootstrap plus service cases for the gesture rule and stale areas (100 across the two).
  • Library: content-zoom-root.component.test.tsx (7), the unit project, and npm run build through typedoc, with a fresh build leaving the committed dist/ unchanged.
  • Extension: a jsdom render test for both footnotes pane positions, a source-contract test pinning the two main markers and the untouched scroll container, and a JSON test for the new menu group and strings.
  • E2E, headless, one spec per invocation: content-zoom.spec.ts 1 passed (independent wheel zoom over text and footnotes, fixed toolbar height, indicator text and data-area, command path, per-area reset via a real click, shared memory identity with the read-only editor, memory keys); content-zoom-default-level.spec.ts 1 passed (seeded 120 % default). The scripture-editor isolated subset: 8 passed, 2 skipped (the gated chord file), 2 failing on a first-run tour dialog that fails identically on the base branch under the same invocation.
  • Review passes: every work item ran /review-paratext (four analyzer passes) and an OpenCodeReview delegate pass, and roborev per-commit reviews were triaged on every commit. No Critical finding is open.
  • Manual checks pending: Settings order, the live stepper, both modes and Spanish; the Power editor tab, a dialog tab, a Simple Column-3 tab, the keyboard path; caret accuracy and popover anchoring at 150 %, the Simple-mode character-marker bar off 100 %, and the spinner, book-not-available and empty-chapter views inside the new wrapper.

Risk Level

Medium. It removes an app-wide shortcut, changes the tab title component every tab renders in both modes, relabels two visible settings, and touches two platform paths that every marked view will share.

AI-assisted — Claude Code (Fable 5.1 coordinating; Opus design, investigation and review passes; per-task Sonnet agents under TDD; Sonnet verification passes).

🤖 Generated with Claude Code

Review round 1 (2026-09-16)

Fifteen commits answer @lyonsil's review; the branch was also restacked onto #2803's rebased tip (1bdd333). Four "Decisions worth a look" bullets above are superseded here:

  • The dialog gate is modal-only and applies only where a pane has to be guessed. isAnyDialogOpen became isModalOverlayOpen (a modal dialog or the command palette); a docked non-modal dialog such as About blocks nothing, and resolveContentZoomTarget gates only the no-id path, so in-iframe chords, the wheel, the tab menu and explicitly targeted calls are never blocked (65a95458fb8). The chrome listener keeps its own explicit modal check after the key filter, and consumes a chord only when something can zoom (17b1cf03c8c, b22d332035b).
  • Tab-menu zoom items are shown disabled on a pane with no zoom area, read synchronously from the resolver when the menu opens, in both modes (1bc7c7e63c0). The remaining edge is a menu opened inside the ~1 s mount grace of a freshly opened pane, which can grey items that would work a moment later. This also fixes a latent gap: renderTabMenuItems previously ignored item.disabled for every contributed tab-menu item.
  • The click-gesture rule is bounded by Tab. A primary click still protects its area against the view's own answering refocus for 200 ms, but a Tab keydown ends the gesture, so a deliberate focus move retargets zoom at once; a right- or middle-click sets the area active but arms nothing (898294115de, b379382b791). By design §2.5 decision 1, a chord follows the caret while the menu follows the last click or focus, so after clicking a footnote row the two can act on different areas until the next focus move; the ADR and the catalog say so now.
  • The keyboard-chord e2e cases run. Both stated blockers were gone; the file is unskipped and the footnotes case moves keyboard focus onto a footnote row before pressing the chord (77b2e83f900, ce167766772). Both cases pass on the first attempt.

Also in this round: the chrome listener registers in the capture phase so the reference box's picker cannot swallow the chords (17b1cf03c8c); each web view type's contributed tab menu is read once per process, so an extension installed mid-session shows its tab items at the next window reload (85f5991f70c); the Settings stepper's buttons use aria-disabled so they keep focus and their tooltip at a bound (0f68f04159b, b22d332035b); the debounced settings handler is memoized (e67b4efe879); the macOS menubar test clears its shared mock so the numpad cases can fail (fb70d1c0cac); and the ADR, the Extension Development Guide, the platform.zoomIn/zoomOut summaries and one Spanish string are corrected (ca75d2fd47b, cfc626960d5).

Scope reduction (product decision 2026-09-16): PT-4578 asked for the Simple-mode tab menu on the editor and Home tabs too. The headless Column 1/2 tab bars keep pointer-events: none; the editor's hamburger menu is the Simple-mode entry point, and the tab menu is delivered on Column 3 alone. Recorded in adr-simple-mode-tab-menu-offers-zoom-only.

Not taken here, on purpose: interface scaling stays Settings-only with the two commands callable and shortcut-less (PO decision 8, PRD NN-5, PT-4577); the 10 % snap on platform.zoomIn/zoomOut is ticket-ordered; the per-notch rescan and forced layout during Ctrl+wheel, and the nested-marker selector, live on #2803's branch and are handled there; the mode-switch dialog-request leak is fixed on its own branch off main (dialog-request-survives-layout-load); the macOS menu-combiner shallow copy is proposed as a follow-up ticket.

Battery on the tip: typecheck (core, erb, e2e, workspaces) 0 errors; lint 0 errors; format:check clean; vitest over the renderer services, docking, settings, main and stories trees 135 files / 2451 passed; content-zoom-chords.spec.ts 2 passed headless.


This change is Reviewable

Round 2 (review by lyonsil, 2026-09-16)

Forty-two commits answer @lyonsil's second review (two passes, 25 inline findings plus two that could
not be attached to a line). The branch was restacked onto #2803's rebased base (ca92e777ccf), and
then, after #2803 squash-merged into main (99b58a037aa), restacked a second time onto main; it now
carries 102 commits above main.

Chords. The zoom chords are declared once, in CONTENT_ZOOM_CHORDS in content-zoom.model.ts,
and the window-chrome listener, the in-view bootstrap and the macOS View menu are all built from that
one table — the two hand-kept copies that made the next two bugs possible in two places at once are
gone. Shift is accepted for every zoom chord rather than for zoom-in alone, so Ctrl+0 resets on
AZERTY and Czech layouts, where 0 is a shifted key and reset had no other keyboard route. The reset
chord's Numpad0 entry now requires key: '0', so with NumLock off the keystroke is Ctrl+Insert
and reaches Copy instead of being swallowed. The macOS View menu's accelerator is ⌘= with a hidden
⇧⌘= duplicate (⌘+ and ⌘= are different native key equivalents; the catalogue claimed the wrong one),
and the accelerators are asserted as literals rather than read back out of the table they come from.

Settings stepper. The stepper writes through the debounced handler like every sibling control, on
a 150 ms wait rather than the typed controls' longer one, so a burst of presses is one cross-process
write and one fan-out instead of one per press. It steps with the platform's own adjustZoomFactor
rather than a fourth copy of the arithmetic, which let min, max, step, roundToStep and
clampToProps go; with the generality gone it is renamed PercentStepper → ZoomStepper. Buttons,
readout and reset gates all derive from the same baseline, so the reset button no longer refuses the
click that its own stale aria-disabled should have allowed, and a burst keeps its place when an
older press is confirmed mid-burst. The bound tooltips say why a press does nothing ("Already at the
largest zoom (300 %)", the mirror at 50 %, and the reset button's at-default wording), in English and
Spanish. Two failures the optimistic readout exposed are fixed with it: a change with no writer is now
an error rather than a silent success, and a failed settings write is logged rather than existing only
as component state.

The window-input gate. ConnectionLostOverlay, WorkspaceUpdatingOverlay and FirstRunOverlay
bypass the overlay store entirely, so the chord gate did not stand down behind them and a chord could
zoom — and persist — a pane the user could not see. They now register through one hook into a
renderer-level store of blocking surfaces, keyed by token; the registration is a layout effect,
because a passive one commits after paint and leaves one frame in which the scrim is up and the gate
still answers "unblocked". The predicate is renamed isModalOverlayOpen → isWindowInputBlocked,
since "modal overlay" is no longer what it means.

Tab menu. The contributed tab-menu cache is keyed by interface mode as well as web view type, so
the first mode-gated tab item to land will not inherit a cache that cannot see a mode switch.

macOS menubar. The combiner mutated the module-level template's submenu arrays, so the first build
that merged a contributed column permanently corrupted the template for the process. It now builds a
per-entry copy that copies the submenu array (not structuredClone, not a JSON round trip — the zoom
items carry click closures). The same entry point had a second defect: a contributed column whose
label matched replaced the platform submenu outright, so that menubar shipped with no zoom items, no
reload, no dev tools and no full-screen toggle; contributed items are now interleaved with the
platform's by order. This closes PT-4633 in full.

Ctrl+wheel cost, measured and fixed. The per-notch forced layout was recorded only on PT-4581,
which closes with this PR, so we captured it before merge. Chrome tracing over a 3 s synthetic
Ctrl+wheel burst at 100 notches/s over the Scripture editor's text found ~1.46 s of a 3.58 s gesture
spent on style and layout forced synchronously inside the wheel handler, with 80 tasks over 16.7 ms.
Controls settled the attribution: the same gesture without Ctrl left the main thread 0.9 % busy, and
150 % and 100 % were indistinguishable, so it was the zoom mechanism, not the editor and not the zoom
level. Two commits fix it. The badge's placement (its getBoundingClientRect and getComputedStyle
reads) moved to one requestAnimationFrame per frame; re-capturing then showed the write itself was
the other half, so the zoom write is coalesced per frame too, through the bound helper's existing step
count — no message shape changed. Headline numbers across the three captures at 150 %: forced style +
layout inside the wheel handler 1459 ms → 0 ms; mean wheel dispatch including everything nested in
it 9.37 ms → 0.35 ms (worst 33.1 → 18.3 ms); style/layout passes over the gesture 299 → 147;
scripting + style + layout 61.0 % → 57.3 % of the window. Stated plainly because it is not fixed:
frame drops are not. Long tasks rose, because the write and the style and layout it forces now land in
the same frame task, and under this load Chromium delivers roughly one wheel event per frame, so
per-frame coalescing has reached its floor. The neighbouring pinch path was measured rather than
assumed: 536 adjust calls in a brisk pinch cost about 0.06 ms each and collapse into the one recalc
their frame owes anyway, so it needs no coalescing of its own. All of these numbers go on PT-4581.

Docs. The platform.zoomIn/zoomOut summaries and TSDoc say "by 10 %, stepping from the nearest
10 %" and carry #2803's naming ruling (interface scaling / tab content default zoom); the ADR records
what giving up the app-wide chords costs a Windows or Linux user, and then corrects itself — Enhanced
Resources answers all three chords from its own handler, so Notes and the Text Collection, not
Resources, are the views where they are dead; the tab menu's comment and ADR entry say the zoom items
are gated by a snapshot read at open, not by a subscription; the editor's zoom-area comment says what
the area actually contains; and a one-test-per-file rule the project does not have is dropped from two
e2e headers.

The restack

git rebase --onto ca92e777ccf 1bdd333eb06, with rerere disabled on every invocation. Seven
conflicts, all in this PR's own files; the saved diffs and the per-conflict resolutions are recorded
in the review notes. The two that changed behaviour rather than prose: the localization relabel took
the base's key layout (%settings_platform_zoomFactor_label_2% = "Interface scaling", the old key
deprecated) and dropped our in-place rename of the same string, and the platform.zoomFactor TSDoc is
the union of both sides rather than either one.

Two judgement calls in the wheel-handler merge, for the base owner. #2803 rewrote the whole wheel
path (tick accumulator with a carried remainder, a pinch branch with physical-modifier evidence, a
line/page-mode branch) while this branch replaced the single act(...) that path ended in. The base's
block is kept verbatim and the coalescer re-applied on top of it, which meant deciding (a) that only
the tick path is coalesced
— the pinch and line/page branches stay immediate, since a pinch already
arrives once per frame — and (b) that the base's seven per-notch assertions are re-expressed as one
coalesced write
carrying the steps the notches add up to, rather than as N single writes. A
subsequent pass corrected two things in that merge: a burst that reverses direction inside one frame
is no longer netted (at a bound the clamp absorbs travel the area cannot take, so a net would hand it
back — each direction now gets its own write, landing where the same notches land one at a time), and
a line/page-mode event or a pinch hands over any notches pending in that frame before it acts.

The second restack, onto main after #2803 squash-merged (99b58a037aa), had one conflict: the
committed platform-bible-react bundles, resolved by taking main's side and then rebuilding the
whole bundle set (f4002416b48). It also surfaced one semantic breakage — main's new middle-click
dock-layout contract test reads the tab title through useInterfaceMode as well as
useIsPowerMode, and the unmocked hook reached the real useSetting, so the test needed the
interface-mode hook mocked too (5ae33da1a1c).

Known, deliberate, and open

  • Hidden panes and the per-frame wheel coalescing (cross-view-sync rule, hidden case). A wheel
    burst's pending steps are applied by a requestAnimationFrame callback inside the web view. In a
    pane whose tab is inactive (rc-dock keeps it mounted but display: none) no frame fires, so the
    steps wait and land whole, in one write, when the pane next paints; nothing is dropped and nothing
    is applied out of order. A hidden pane cannot receive a wheel event in the first place, so the only
    way to reach this is a burst that ends as the tab is switched away. Stated in the in-view script's
    docblock and here.
  • In-view chords are not gated by the full-screen covers, and no longer need to be.
    resolveContentZoomTarget answers a caller that names a pane whatever is on top; only the guessing
    path stands down, and the gate fix above covers the window-chrome listener. A live check of what
    that leaves — focus sitting inside a web view while a cover is up — found the one cover that did
    not hold: connection-lost and first-run are Radix modals and trap focus, but the "Updating project
    view" cover was a plain role="status" div, so the iframe kept the keyboard and a chord typed into
    a pane nobody could see still zoomed it and persisted the level. That cover is now the same Radix
    modal dialog the other two are: it takes focus onto itself when it appears, keeps Tab inside it,
    and hands focus back to whatever held it when the switch ends (7da5b9cfbdd). So all three
    covers trap focus, and no chord reaches a web view behind any of them.
    Pinned by three tests in
    overlay-workspace-updating.component.test.tsx — focus leaves the web view, focus comes back, and
    the app behind the cover is aria-hidden (the same containment proxy the connection-lost suite
    uses, since jsdom cannot drive real Tab containment).
  • Pop-ups that portal out of a zoom area still render at 100 % — the Simple-mode character-marker
    bar's menu, the marker menu and the comment editor. Making the platform-bible-react pop-up
    primitives follow the ContentZoomRoot they came from is PT-4634 on PT-4584: Content zoom for the comment list and the Comments panel #2825, which sits directly
    above this PR; the two non-descendant cases get their one-line opt-in there. If this PR merges
    alone, those pop-ups stay at 100 % until PT-4584: Content zoom for the comment list and the Comments panel #2825 merges. The "Simple-mode character-marker bar off
    100 %" item in the manual-checks list above is therefore re-pointed at PT-4584: Content zoom for the comment list and the Comments panel #2825.
  • PT-4641 filed (Dev Task under PT-2161, not started): both useMemo'd debounces in
    other-setting.component.tsx and project-setting.component.tsx are rebuilt on every render,
    because each builds its validator as a fresh inline arrow, so an armed timer fires later with a
    stale closure instead of being cancelled. "One write per burst" holds only while that prop's
    identity is stable. The root cause is in two files this PR does not touch.

Battery on the tip: typecheck (core, erb, e2e, workspaces) 0 errors; lint 0 errors; format:check
clean; build:types leaves papi.d.ts unchanged; 23 affected suites 541/541, and test:core 321
files / 5359 passed / 0 failed at the restacked tip; the content-zoom e2e specs pass headless.

Trailer note: thirteen of this round's commits carry a Co-Authored-By: Claude Opus 5 (1M context)
trailer alongside the coordinating session's own.

@lyonsil lyonsil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: per-pane content zoom

A review of the commits above pt-4576-content-zoom-platform-core — chord ownership and the macOS View menu, the ContentZoomRoot opt-in contract, the tab menu, the Settings stepper, and the Scripture editor adoption. It raises 32 findings: 27 are attached to the lines they concern, and the 5 below could not be attached because they sit on lines this PR does not change.

Each comment carries the severity the review settled on, plus one of two labels. checked and confirmed means the finding was verified against the code — the reasoning behind it is included under each one. needs a human call means it could not be settled either way and the judgement is yours.

Findings that could not be attached to a line in this diff

#12 (medium · checked and confirmed) src/renderer/services/web-view-content-zoom.bootstrap-script.ts:228 - Showing the zoom indicator re-triggers a whole-document rescan of the editor, so every zoom step pays for two full scans instead of one.

What happens: the MutationObserver watches document.documentElement with {childList: true, subtree: true} (bootstrap-script.ts:228) and its callback ignores the records, running collectAreas() — a full-document querySelectorAll plus a parentElement.closest(...) walk per hit (:184-193). ensureIndicatorElements appends the badge inside that subtree, and badge.textContent = text (:440) is a replace-all operation, so it emits a childList record and re-fires the observer in the same frame. collectAreas() runs before the identical-list comparison, so an unchanged list does not short-circuit the scan.

Why it matters: during a Ctrl+wheel burst each notch pays cornerOf's scan and one more from its own badge update, and every unrelated editor DOM change — footnote popover, chapter navigation, decorations — pays a scan too.

Fix: have the observer callback read its MutationRecord[] and skip collectAreas() unless a record touches the marker attribute or adds/removes a marker; exclude the two indicator nodes by id.

How this was checked: Verified precisely: calling showIndicator once produced 1 querySelectorAll('[data-platform-content-zoom-root]') call synchronously (cornerOf's own scan), then a second one after the next microtask flush — the exact point where a MutationObserver callback fires — with no further increase after that. A second, independent observer on document.body confirmed the second call lines up with a childList mutation record targeting the indicator badge itself, so badge.textContent = text (bootstrap-script.ts:440) is self-triggering refresh(), i.e. collectAreas() (:205-206) runs a second full-document scan plus a .closest() per hit for every indicator update. This confirms the DOM-spec point: textContent's setter is "replace all", which always removes and inserts nodes (a childList record) rather than patching an existing text node's data (a characterData record the {childList:true} config would not see). It also confirms that collectAreas() runs unconditionally before the next.join(...) !== areas.join(...) comparison (:206-210), so an identical area list never short-circuits the scan itself, only the report to the parent. Net effect during a Ctrl+wheel burst: every notch pays for cornerOf's scan and one more full-document collectAreas() from its own badge update. The editor had zero markers to scan before this PR's ContentZoomRoot additions, so this was dormant there until now.

Not attached to a line: no longer on a changed line; cannot be posted inline

#15 (medium · checked and confirmed) src/renderer/services/web-view-content-zoom.bootstrap-script.ts:427 - Ctrl+wheel over a Scripture chapter forces a style recalculation and layout read on every single wheel notch.

What happens: pushContentZoom writes the zoom custom property on documentElement and then synchronously calls showIndicator, whose cornerOf (bootstrap-script.ts:376-390) runs a whole-document querySelectorAll('[data-platform-content-zoom-root]') plus a getBoundingClientRect() per marker — a style-write-then-geometry-read pair — and allocates a fresh window.matchMedia(...) at :447. Nothing upstream throttles it. Before this PR the editor reported no areas, so this never ran there; platform-scripture-editor.web-view.tsx:3827 is what turns it on.

Why it matters: trackpad wheel events arrive at 60-120 Hz until the 0.5-3.0 clamp, and keyboard auto-repeat takes the same path.

Fix: keep badge.textContent synchronous, but move cornerOf and the position writes into a requestAnimationFrame that collapses repeats, and hoist the MediaQueryList to bootstrap time.

How this was checked: Verified by instrumented jsdom run (the repo's own install() harness plus spies on document.querySelectorAll and Element.prototype.getBoundingClientRect): with 20 simulated wheel notches, cornerOf ran exactly 20 times — one full-document querySelectorAll('[data-platform-content-zoom-root]') plus one getBoundingClientRect() per notch — and dispatching 15 real wheel events with ctrlKey: true produced 15 calls into the bound adjust function, a clean 1:1 with no debounce anywhere in the chain (the bootstrap's onWheel, web-view.service-shard.ts's adjustContentZoomById, or adjustContentZoom/pushContentZoom; the only debounce there, setOwnLevels's OWN_LEVEL_WRITE_DEBOUNCE_MS, gates the definition write, not the pushContentZoom(target, {indicator}) call, which fires unconditionally on every notch). web-view-content-zoom.service.ts:568 also clears iframe.style.zoom before writing the per-area CSS variable, so the write-then-getBoundingClientRect() pair in cornerOf is a real forced-synchronous-layout pattern, not just a cheap read. On framing: before this PR the Scripture editor reported zero areas, so targetFor returned undefined and onWheel/onKeyDown short-circuited before ever reaching cornerOf; this PR's ContentZoomRoot mounts (platform-scripture-editor.web-view.tsx:3823 and :3958) are what make this per-notch path run in the editor for the first time. Measured as call counts in jsdom, which does no real layout.

Not attached to a line: no longer on a changed line; cannot be posted inline

#24 (low · checked and confirmed) src/main/platform-macos-menubar.util.ts:213 - An extension that contributes a "View" main-menu column permanently deletes the macOS zoom accelerators for the rest of the session.

What happens: const combinedMenubar = [...macosMenubarObject]; (platform-macos-menubar.util.ts:198) is a shallow copy, so existingMenu.submenu = column.submenu writes through to the module-level element. The match is on the unlocalized label, and the View entry's label is literally %mainMenu_view% (platform-macos-menubar.data.ts:90). Every later rebuild — and fallbackToDefaultMacosMenubar — then starts from the clobbered object.

Why it matters: mainMenu.columns is isExtensible: true, but nothing in-tree contributes a View column, so only a third-party extension triggers it. The mutation itself is pre-existing and until now harmless — the Help entry it already clobbers every session had an empty submenu. This PR is what first puts something worth losing behind it: the app's only macOS binding for Cmd+=/Cmd+-/Cmd+0.

Fix: deep-copy the entries in translatePlatformMenuItemsAndCombine — macosMenubarObject.map((m) => ({ ...m, submenu: Array.isArray(m.submenu) ? [...m.submenu] : m.submenu })) — which also fixes the appMenu.submenu?.push(...) duplicate accumulation at :207.

How this was checked: I reproduced the write-through directly against the real module: temporarily exported translatePlatformMenuItemsAndCombine, ran it via vitest with a synthetic "View" column (label: '%mainMenu_view%'), and confirmed macosMenubarObject's View entry had its submenu overwritten to [] in the live module — a second, unrelated combine call afterward still returned the clobbered empty submenu, proving the damage persists for the rest of the session. I also separately confirmed the appMenu .push(...) duplicate-accumulation bug is real: three combine calls grew appMenu's submenu length by exactly one item each time (3 -> 4 -> 5 -> 6), with no dedup. Both mechanisms live entirely in src/main/platform-macos-menubar.util.ts, which this PR does not touch, so the mutation itself is pre-existing. What this PR changes is platform-macos-menubar.data.ts: it puts real content — the only macOS Cmd+=/Cmd+-/Cmd+0 bindings — into the object this pre-existing code can clobber. I checked every in-tree menus.json under extensions/ plus src/extension-host/data/menu.data.json: only platform.app and platform.help are contributed mainMenu columns, so this fires only if a third-party extension adds a View column. I also confirmed the identical mechanism already fires every session for the Help entry with zero observable harm, because Help's submenu was already [] before this PR — there was nothing there to lose.

Not attached to a line: no longer on a changed line; cannot be posted inline

#25 (low · checked and confirmed) src/renderer/components/docking/dock-layout-wrapper.simple-mode.scss:56 - In Simple mode the new tab zoom menu cannot be opened on the Home or Scripture-editor tabs.

What happens: Simple mode puts columns 1 and 2 in HEADLESS_GROUP (simple-layout.data.ts:62,87; platform-dock-layout-positioning.util.ts:94-95 maps SCRIPTURE_EDITOR_WEBVIEW_TYPE there), and the headless tab bar is pointer-events: none with nothing re-enabling it on a child. A right-click never reaches ContextMenuTrigger, so the tab menu is reachable only on column-3 tabs.

Why it matters: the zoom actions are not lost — platform-scripture-editor.web-view.tsx:3768-3771 renders TabToolbar unconditionally in both modes, and the platformScriptureEditor.zoom group carries no hiddenInterfaceModes, so the editor's hamburger carries them. The ADR adr-simple-mode-tab-menu-offers-zoom-only records this arrangement deliberately. The SCSS is untouched by this PR.

Fix: none required for the editor. If the Home column should also offer zoom, give it an entry point; otherwise leave the ADR as the record.

Not attached to a line: no longer on a changed line; cannot be posted inline

#27 (low · checked and confirmed) src/renderer/components/docking/platform-tab-title.component.tsx:967 - Every mounted tab rebuilds its context-menu item elements on every focus change anywhere in the window.

What happens: handleSelect is a fresh closure each render (platform-tab-title.component.tsx:936) and renderTabMenuItems(tabMenuItems, handleSelect) is called inline at :967, so the children value is rebuilt on every render — and the file's own comment at :868-871 records that one focus change re-renders every mounted tab title.

Why it matters: the cost is small, not a DOM rebuild: ContextMenuContent passes no forceMount, so Radix's Presence gating means the subtree is never mounted while the menu is closed, and the discarded work is a .map over a handful of elements. The pattern is also pre-existing for Power-mode tabs; this PR extends it to Simple-mode tabs that now have items.

Fix: wrap handleSelect in useCallback and the rendered items in useMemo keyed on tabMenuItems, in platform-tab-title.component.tsx.

Not attached to a line: no longer on a changed line; cannot be posted inline

(AI-assisted, with my guidance)

Comment thread .context/standards/Architecture-Decisions.md Outdated
Comment thread .context/standards/Extension-Development-Guide.md Outdated
Comment thread assets/localization/en.json
Comment thread e2e-tests/tests/isolated/scripture-editor/content-zoom-chords.spec.ts Outdated
Comment thread src/renderer/services/web-view-content-zoom.chrome-keys.ts
Comment thread src/renderer/services/web-view-content-zoom.chrome-keys.ts Outdated
Comment thread src/renderer/services/web-view-content-zoom.service.ts Outdated
Comment thread src/renderer/services/web-view-content-zoom.service.ts
Comment thread src/shared/data/keyboard-shortcuts.data.ts
@rolfheij-sil
rolfheij-sil force-pushed the pt-4576-content-zoom-platform-core branch from 554b2a9 to 1bdd333 Compare September 16, 2026 11:40
rolfheij-sil added a commit that referenced this pull request Sep 16, 2026
…ode scope

PT-4577/PT-4578 (PR #2821) landed; the docs sweep folds the shipped
behavior back into the ADRs, the extension guide and the two public
summaries/TSDoc that still described a pending state:

- adr-web-view-content-zoom-in-iframe-shortcuts: the chord-ownership
  handoff from main is now past tense, and its Source cites #2821.
- adr-simple-mode-tab-menu-offers-zoom-only: Source cites #2821, and
  Consequences records the 2026-09-16 product decision narrowing the
  tab-menu route to Simple mode's one visible, interactive tab bar
  (Column 3) rather than the editor's headless one.
- Extension-Development-Guide.md: the Interface-scaling/Tab-content-
  default-zoom labels are stated as shipped, and a new bullet notes
  Simple mode's tab menu is narrowed to platform.tabZoom.
- platform.zoomIn/zoomOut: the OpenRPC summary and TSDoc now say they
  zoom menus, toolbars AND content, matching PT-4577's DoD wording;
  papi.d.ts regenerated to match (TSDoc only, no signature change).
- es.json: mainMenu_view_resetZoom now reads "Restablecer el zoom...",
  matching the majority Spanish phrasing already used elsewhere.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
@rolfheij-sil

Copy link
Copy Markdown
Contributor Author

🤖 Claude: answers to the five findings that could not be attached to a line.

#12 — "Showing the zoom indicator re-triggers a whole-document rescan of the editor, so every zoom step pays for two full scans instead of one." The mechanism is confirmed: the observer's attributeFilter narrows attributes only, childList records anywhere still fire it, badge.textContent = text is a replace-all and so emits one, and collectAreas() runs before the identical-list comparison rather than after it. The fix site is not in this PR, though: the observer registration and showIndicator are on #2803's branch (pt-4576-content-zoom-platform-core), untouched here, so this goes to that PR along with your write-up. One thing worth handing over with it — the implementation plan for that work required the observer refresh to be rAF-coalesced, and the shipped bootstrap dropped that hop deliberately (to clear the hidden whole-iframe fallback before first paint), so the fix has a documented starting point and a documented reason to be careful. Magnitude is still unmeasured: both your run and ours counted calls in jsdom, which does no layout. What would size it is a Chrome DevTools performance capture over a ~2 s Ctrl+wheel burst on a Scripture editor pane, reading the "Recalculate Style" and "Layout" totals.

#15 — "Ctrl+wheel over a Scripture chapter forces a style recalculation and layout read on every single wheel notch." Same verdict and the same home. The write-then-getBoundingClientRect() pair is real, pushContentZoom clears iframe.style.zoom before writing the per-area variable, and nothing in the chain throttles: the only debounce gates the definition write, not the per-notch pushContentZoom(target, { indicator }). Because it shares a fix site with #12 — both inside the serialized bootstrap on #2803 — the two go over as one work item, with the same rAF note and the same measurement. We are not taking the matchMedia hoist sight-unseen: the per-call allocation is commented as deliberate, so that half needs its own decision on that branch rather than being folded in.

#24 — "An extension that contributes a 'View' main-menu column permanently deletes the macOS zoom accelerators for the rest of the session." Confirmed, including the appMenu.submenu?.push(...) accumulation, and confirmed as pre-existing: platform-macos-menubar.util.ts is not touched by this PR, and nothing in the tree contributes a %mainMenu_view% column, so there is no trigger today. You are right that what this PR changes is that the object now holds something worth losing. We are not taking it here — it is a main-process bug unrelated to zoom, and the question is placement rather than size. It is proposed as a follow-up ticket; there is no key yet, so please read that as a proposal rather than a commitment, and we will post the key here if and when one exists.

#25 — "In Simple mode the new tab zoom menu cannot be opened on the Home or Scripture-editor tabs." Confirmed, and the scope is now formally narrower rather than the shipped state being an accident. Product narrowed PT-4578 on 2026-09-16: the tab-menu route to content zoom is required only where Simple mode already has a visible, interactive tab bar — Column 3 — and the editor's own hamburger is its sole Simple-mode entry point, which matches your "none required for the editor". That decision is recorded in the adr-simple-mode-tab-menu-offers-zoom-only Consequences (ca75d2fd47b) and called out in the PR body. The headless Column 1 and 2 tab bars keep their pointer-events: none; this feature does not make them interactive to reach parity. The Home column is not getting an entry point in this PR either; it stays on the UX follow-up list.

#27 — "Every mounted tab rebuilds its context-menu item elements on every focus change anywhere in the window." Accurate, and declined. Your own analysis is most of the reason: ContextMenuContent passes no forceMount, so Radix's Presence keeps the subtree unmounted while the menu is closed, the discarded work is a .map over a handful of elements behind that gate, and the pattern is pre-existing for Power-mode tabs. The other half is that the hoist is not the two-liner it looks like: handleSelect is declared after the component's early return for a tab with no items (if (tabMenuItems.length === 0) return titleWithTooltip;), so a useCallback cannot go where it is without moving above that return and restructuring the return around it. With no measurable cost on the other side of the ledger, we would rather leave the shape alone.

@rolfheij-sil

Copy link
Copy Markdown
Contributor Author

🤖 Claude: follow-up on #24 — the macOS menu-combiner shallow copy now has a ticket: PT-4633, a sub-task of the zoom epic PT-4575. The dialog-request leak from #4 is PR #2828, now ready for review.

@lyonsil lyonsil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 2: what round 1 left open

A re-read of the fifteen commits that answer round 1, on 2a2cfd95f9d. The rest of round 1 checks out — each claimed fix is in the code, reverting it fails the test that was written for it, and those threads are now resolved. Three items are not closed, and they are below.

Each comment carries the severity this review settled on plus the label checked and confirmed, meaning the finding was verified against the code; the reasoning behind it is included under each one.

Findings that could not be attached to a line in this diff

#3 (low · checked and confirmed) src/renderer/services/web-view-content-zoom.bootstrap-script.ts:394 - The per-wheel-notch forced layout is recorded only on PT-4581, the work item this PR delivers, so it closes with the PR.

What happens: pushContentZoom writes each area's zoom variable and then calls the bootstrap's showIndicator synchronously (web-view-content-zoom.service.ts:566-598), whose cornerOf (bootstrap-script.ts:394-409) runs a whole-document querySelectorAll plus a getBoundingClientRect() per marker, and whose :464 allocates a fresh matchMedia. Nothing in the chain throttles it. The hand-off routes this PR to #2803, and #2803 routes it to PT-4581's "Wheel-notch layout cost" bullet - added 2026-09-16, reading "measure it here with the editor's real content". PT-4581 is the work item this PR delivers, and the PR's own "Manual checks pending" list carries no DevTools capture.

Why it matters: the Scripture editor is the first view to mark areas, so this write-then-read pair runs in the editor for the first time in this PR, at trackpad wheel rates of 60-120 Hz. Merging PT-4581 closes the only record that the measurement is owed.

Fix: give it a sub-task of its own under PT-4575, as the macOS menu-combiner finding got in PT-4633, or take the capture before merge and record the number on PT-4581.

How this was checked: The mechanism is unchanged on this head: pushContentZoom (web-view-content-zoom.service.ts:566) sets the per-area CSS variables and then calls showIndicator at :598, and cornerOf at bootstrap-script.ts:394-409 still scans the document and reads getBoundingClientRect() per marker, with a fresh matchMedia at :464; there is no rAF hop and no throttle anywhere between the wheel listener and that pair. For the tracking half I read PT-4581's changelog: the "Wheel-notch layout cost" bullet was added to its description on 2026-09-16 at 07:42, and PT-4576 carries no equivalent bullet - so the record lives only on the work item this PR delivers.

Not attached to a line: no longer on a changed line; cannot be posted inline

(AI-assisted, with my guidance)

Comment thread src/main/main.ts Outdated
Comment thread src/renderer/services/window-input-blocked.util.test.ts

@lyonsil lyonsil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another review pass over the commits above pt-4576-content-zoom-platform-core, at 2a2cfd9.

21 findings: 20 are inline on the lines they concern, and one is below because it sits on code this PR did not change. Each comment leads with the severity settled on after checking the behaviour against the code at this head, and checked and confirmed means it was verified there rather than inferred. Where several comments share one fix, each names its siblings — the fix has to land at every site.

Findings that could not be attached to a line in this diff

#21 (low · checked and confirmed) src/renderer/components/docking/platform-tab-title.component.tsx:1062 - Simple-mode tabs now rebuild a menu item tree on every render, including on every focus change.

What happens: handleSelect is a plain const (platform-tab-title.component.tsx:1031), a fresh closure each render, and renderTabMenuItems(tabMenuItems, handleSelect) is called inline in the JSX at :1062 with no memo around either. The comment at :966 records that a single focus change re-renders every mounted tab title, because the focus subscription, useLastFocusedTabId and useLastSelectedScriptureNavigableWebViewId all fan out to every instance.

Why it matters: both the unmemoized rebuild and the focus fan-out are pre-existing — identical at the base branch. What changed is who reaches them: Simple mode's tabMenuItems used to be hard-coded empty and an early return skipped this code entirely, and Column 3 tabs now build the tree for the first time. That tree is three flat ContextMenuItems from the platform.tabZoom group, with no separators or submenu.

Fix: wrap handleSelect in useCallback and the rendered items in useMemo in platform-tab-title.component.tsx. Worth doing while the file is open rather than on its own.

Not attached to a line: no longer on a changed line; cannot be posted inline

(AI-assisted, with my guidance)

Comment thread src/main/main.ts
Comment thread src/renderer/services/modal-overlay-open.util.ts Outdated
Comment thread src/renderer/services/web-view-content-zoom.chrome-keys.ts Outdated
Comment thread src/renderer/services/web-view-content-zoom.chrome-keys.ts Outdated
@rolfheij-sil
rolfheij-sil force-pushed the pt-4576-content-zoom-platform-core branch from 1bdd333 to ca92e77 Compare September 17, 2026 22:14
rolfheij-sil added a commit that referenced this pull request Sep 18, 2026
Adds the platform half of per-pane content zoom (work item 1 of epic
PT-4575). A web view marks one or more zoom areas with the
data-platform-content-zoom-root attribute (no value, "main" or "true"
for the main area; a name for a named area; a marker nested inside
another is ignored). The platform keeps a level per area in the pane's
saved definition state, remembers it per project and kind of pane in the
hidden platform.webViewContentZoomMemory setting, pushes the effective
factors into the pane as --platform-content-zoom-<area> CSS variables,
shows an on-area indicator with a live-region announcement, follows the
platform.webViewContentZoom default for areas without their own level,
keeps sibling panes of the same project in step across windows, and
scales URL views and views that never report an area whole at the
default after a one-second grace.

Input arrives through a bootstrap script injected into every non-URL
web view and through three owner-routed commands
(platform.webViewContentZoomIn/Out/Reset). Ctrl/Cmd+wheel counts wheel
ticks the way Chromium's own page zoom does (wheelDeltaY / 120 with the
remainder carried), so one notch is one step on Windows, macOS and
Linux; a trackpad pinch, which Chromium synthesizes as ctrl+wheel, is
recognized separately (ctrlKey with no physical modifier seen by key or
pointer events, small scale change, latched per area) and steps by
accumulated scale.

Robustness a maintainer should know about: memory writes are debounced
and serialized, retried three times, and a given-up write ignores only
the stale echo of the value it failed to replace; a level whose
definition write failed is still shown and shared while it stays
pending; the zoom-area observer rescans only on mutations that add,
remove or retag a marker; the window subscribes to settings and
registers its unload flush before awaiting its startup reads; teardown
paths log instead of throwing. All new API is marked @experimental on
both the TSDoc and OpenRPC surfaces, and papi.d.ts is regenerated.

Settings labels follow the wording agreed with UX (PT-4579): "Interface
scaling" for the whole application (new localization key, the shipped
one deprecated) and "Tab content default zoom" for panes, recorded in
the Storybook vocabulary page and the Localization Guide. The Spanish
strings were written in-house and still want a translator's check.

Nothing is user-visible on its own: no bundled view marks an area yet
and main still claims the Ctrl chords on Windows/Linux; PT-4577 and
PT-4581 (PR #2821) deliver the editor's areas and free the chords.
Still owed there: the real-device wheel/pinch capture and the observer
profile with the editor's markers. The Text Collection grid's own
per-event wheel zoom is recorded on PT-4582.

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Session-URL: https://claude.ai/code/session_01T6sSbmNudRaS4XGzULm55v
Session-URL: https://claude.ai/code/session_013D76KhfL9JJtpufRTs4oYr
Base automatically changed from pt-4576-content-zoom-platform-core to main September 18, 2026 07:28
rolfheij-sil added a commit that referenced this pull request Sep 18, 2026
…ode scope

PT-4577/PT-4578 (PR #2821) landed; the docs sweep folds the shipped
behavior back into the ADRs, the extension guide and the two public
summaries/TSDoc that still described a pending state:

- adr-web-view-content-zoom-in-iframe-shortcuts: the chord-ownership
  handoff from main is now past tense, and its Source cites #2821.
- adr-simple-mode-tab-menu-offers-zoom-only: Source cites #2821, and
  Consequences records the 2026-09-16 product decision narrowing the
  tab-menu route to Simple mode's one visible, interactive tab bar
  (Column 3) rather than the editor's headless one.
- Extension-Development-Guide.md: the Interface-scaling/Tab-content-
  default-zoom labels are stated as shipped, and a new bullet notes
  Simple mode's tab menu is narrowed to platform.tabZoom.
- platform.zoomIn/zoomOut: the OpenRPC summary and TSDoc now say they
  zoom menus, toolbars AND content, matching PT-4577's DoD wording;
  papi.d.ts regenerated to match (TSDoc only, no signature change).
- es.json: mainMenu_view_resetZoom now reads "Restablecer el zoom...",
  matching the majority Spanish phrasing already used elsewhere.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
rolfheij-sil added a commit that referenced this pull request Sep 18, 2026
…d-facing comment

Matt's review of PR #2821, findings #22 and #23.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
rolfheij-sil added a commit that referenced this pull request Sep 18, 2026
…y store

Matt's review of PR #2821, finding #11.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
rolfheij-sil added a commit that referenced this pull request Sep 18, 2026
…Y and Czech layouts

On those layouts the top-row 0 and - are shifted keys, so the chord arrives with
shiftKey set and both chord copies threw it away. Reset has no other keyboard
route there. Chromium's own zoom accepts Shift the same way, and nothing in the
app claims Ctrl+Shift+- or Ctrl+Shift+0.

Also updates web-view-content-zoom.bootstrap-script.test.ts, which pinned the old
Shift rule; the round-2 plan's lane-A file list omitted it.

Matt's review of PR #2821, finding #2.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
rolfheij-sil added a commit that referenced this pull request Sep 18, 2026
…ithmetic

Its private clamp-and-round rounded before clamping and followed the caller's step
rather than the platform's, so from an off-tenth factor the two rules disagreed —
0.95 went to 0.8 here and to 0.9 everywhere else. Drops min/max/step with it; the
one caller passed exactly the platform's own constants.

Matt's review of PR #2821, finding #24.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
rolfheij-sil added a commit that referenced this pull request Sep 18, 2026
With NumLock off that key reports itself as Insert, and Ctrl+Insert is Chromium's
legacy Copy chord, which both chord handlers consumed before Copy could run.

The parity table's numpad rows also gain the key each browser really reports
alongside the code, so a row exercises a shape a browser can actually emit.

Matt's review of PR #2821, finding #3.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
rolfheij-sil and others added 29 commits September 22, 2026 21:57
The tab-title cache comment still asserted that both interface modes share one contributed menu — the precise premise the mode-keyed cache disproves, so a reader would simplify the key straight back into the bug.

The chord gate widened to cover the three full-screen overlays, but four places still described it as a modal-overlay-only check: resolveContentZoomTarget's doc, the shortcut catalogue's context prose for all three zoom entries (the catalogue is the published source of truth and this is user-visible in Storybook), and two test names driving the renamed dep.

Also: the chord table is serialized when the script is generated, not at build time; the blocked-window check is no longer only an overlay-map scan; ZoomStepper's range comes from the MIN/MAX constants, not from adjustZoomFactor; and the off-grid step comment claimed a counterfactual that is arithmetically false — round-then-step and step-then-round agree for every positive factor, and 1.25 moves 15 points up but 5 down.

Strips the change narration the forward-facing-comment rule targets: 'still stated', 'rather than two hand-maintained copies', 'stays a prop', 'put a term back', and a hook comment naming a spy the file no longer has.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
…cess

The write sat inside the success branch behind an `if (setSetting)`, after the error
message had already been cleared. With no writer the press wrote nothing, cleared any
error on screen, and — on the zoom stepper — moved the readout and announced the new
percentage through aria-live before reverting 1500 ms later.

The error is now cleared only by a completed write, a missing writer throws into the
existing catch, and the zoom stepper is disabled while it has nothing to write to.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
A rejected write reported only into component state, and `setErrorMessage` is a no-op
once the settings tab has closed — so a Send/Receive `(SR_EDIT_BLOCKED)` rejection, at
least a debounce plus a round trip behind the edit that caused it, could vanish
entirely.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
…it matches

A contributed column whose header matched a platform menu replaced that menu's whole
submenu in the build it landed in. For the View menu that means shipping a menubar with
no zoom items, no reload, no dev tools and no full-screen toggle — and on macOS the View
menu's accelerators are the only menu route to the content-zoom chords.

Contributed items are now pushed into the matched menu's per-build submenu copy and
interleaved by `order`, the same way a contributed app-menu column already is.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
…ult zoom

The other two stepper buttons explain their bound; reset repeated its own name while
aria-disabled at the default, which is the one moment the name answers nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
Every Ctrl+wheel notch ran the indicator's placement synchronously inside the
wheel handler: cornerOf's whole-document querySelectorAll plus a
getBoundingClientRect per marked element, and directionOf's getComputedStyle.
Both are forced reads taken right after the zoom write invalidated style and
layout, so a trace of a 3.6 s burst over the Scripture editor at 150 % spent
~1.46 s on style recalc and layout nested inside the wheel dispatches, with 80
main-thread tasks over 16.7 ms; the same gesture without Ctrl left the main
thread 0.9 % busy. Stubbing getBoundingClientRect alone removed the nested
layout but not the style recalc, so both reads had to move.

A notch now writes the zoom and the badge's text immediately and requests a
placement; one requestAnimationFrame callback per frame places the badge for
whichever area and level the burst settled on. destroy() cancels a pending
frame, and a realm without requestAnimationFrame places inline as before.

Two indicator tests that asserted placement synchronously now flush one frame.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
The comment above the editor's ContentZoomRoot claimed only the text scales, but the Simple-mode character-marker bar renders inline and scales with it; content that portals out (menus, pop-ups) is what stays outside. Answers a review note on PR #2821.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
…er; forward-facing comments

- setting.component.test.tsx: add a regression test in the "failed setting writes" describe —
  a failed validation leaves an error on screen, and a later change that passes validation with
  a real writer clears it. Reword the "a setting with no writer" test's comment to describe what
  the second change does now (passes validation but has no writer, so the error stays) instead of
  narrating what it used to do.
- platform-macos-menubar.util.test.ts: give the contributed View-column fixture item an order
  between two platform items (toggleDevTools and the separator below it) and assert the full
  ordered id sequence of the combined View submenu, pinning that a contributed item is interleaved
  by order rather than appended. Reword the "used to remove" comment to the conditional phrasing
  ("would delete") the production comment already uses.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
Every Ctrl+wheel notch applied its zoom change on the spot, and applying one is
not cheap: the parent writes the area's CSS variable, which restyles the whole
pane, and persists the new level. A gesture delivers 50-120 notches a second and
only the level a frame ends on is ever painted, so a 3 s burst over the Scripture
editor spent ~700 ms of scripting and left the main thread ~68 % busy with 82
tasks over 16.7 ms, most of that servicing levels nobody saw.

A notch now adds a step to a pending total and one adjustment per frame carries
that total: three notches in and one out inside a frame is a single +2 write, the
same end level the notches would have reached one at a time (the parent steps and
clamps through the same helper either way), with no notch lost and no level in
between written. Steps pending for one area are applied before another area
starts accumulating, destroy() drops a burst it has not applied, and a realm
without requestAnimationFrame applies in the handler as before. Keyboard chords
stay immediate - one keystroke is one step.

The bound helper already takes a step count, so a burst travels as one call and
no wire contract changes; the commands carry no count, so that fallback path
repeats the command per step rather than dropping notches.

The badge-placement test now drives its run of levels through showIndicator
directly, since a wheel burst no longer produces more than one show per frame.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
The restack onto the rebased platform-core base merged the base's "Interface scaling" parenthetical with this branch's sentence about the chords, so papi.d.ts follows papi-shared-types again.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
Merging the base's grace-arming sentences with this branch's liveness-probe ones left two doc comments wrapped where Prettier does not wrap them.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
…turns

Coalescing a frame's notches into one net write is exact only in the middle of
the zoom range. At either end the parent's clamp ABSORBS the travel the area
cannot take, so notches back the other way start from the bound - while a net
hands the reversal the absorbed travel back. From the minimum, two notches out
and three back in landed at 60 % where the same five notches one at a time land
at 80 %.

A notch whose direction differs from the pending total now flushes that total
first, the way a notch in another area already did, so each run of one direction
travels as its own write and the burst lands where the notches would have one at
a time. A same-direction burst - every real gesture - still coalesces to one
write per frame, which is what the coalescing was for.

Also corrects three claims the coalescing commit left: the range cap is applied
per frame rather than per event, the net-across-a-turn promise is replaced by
what the code now does, and a requestAnimationFrame callback runs ahead of the
frame's style and layout pass, not after it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
The consequence added for F1.7 said Ctrl+plus/minus/0 now do nothing outside the
Scripture editor. Enhanced Resources answers all three itself, from a keydown
handler on its own window, for its scripture-pane zoom - the shortcut catalogue
has published those three entries all along, and content-zoom.model.ts already
records that the view zooms its pane itself. The Text Collection grid really does
have no keyboard path (its own note defers one to PT-4143), so it stays in the
list of views where the chords are dead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
…y causes

The two tests named for the deep-copy guard asserted the zoom item ids were
present with arrayContaining. Restoring the aliasing bug left both green: the
merge landed in the same round, so a contributed column is now APPENDED to the
matching menu, and a write through the shared template grows it rather than
replacing its items - every zoom id is still there, with the previous build's
contributed item beside it. Only the app-menu length test caught it.

The View-column pair now compares whole sequences: a second build's View menu
against the first's, and the template's own submenu before against after. Both
turn red on the aliasing bug, and neither depends on running before any other
test in the file. The copy's comment described the replace-era failure mode too.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
Two guards this round added survived a mutation of themselves.

The setting error message is cleared only after the write resolves, but the test
written for it hands the component a writer that resolves at once, where clearing
before and clearing after are the same run. A write held in flight tells them
apart: the earlier error stays on screen while it is under way and goes when it
lands.

The window-blocking hook registers from a layout effect so the store cannot
answer 'unblocked' for the frame the scrim is already painted in; nothing pinned
that, and swapping in a passive effect left all 34 overlay tests green. A probe
that reads the store from its own layout effect, mounted after the overlay, sees
the window held only while the registration is a layout effect.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
The window was pinned only from above - both tests that touch it advance 2000 ms,
so any value at or under that passed. Shortening it to 1 ms left all 32 tests
green, though a window under the stepper's own 150 ms debounce would make the
second press of every burst re-derive from a prop that has not caught up.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
…e it

The two immediate wheel branches - a line/page-mode event and a trackpad pinch -
act on the spot, so either one jumped ahead of tick steps that arrived earlier in
the same frame and were still pending in the coalescer. Before the coalescing
every wheel event applied in arrival order; the parent's clamp is not
commutative, so at either end of the zoom range the order two steps arrive in is
the level they land on.

Both branches now hand over whatever is pending before they act, the way a notch
for another area and a notch that turns round already do.

Also records the hidden-pane case the coalescer has: an inactive rc-dock tab's
iframe is display:none, where no animation frame runs, so a total pending when
the tab is hidden waits there and lands whole when the tab is shown again.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
…antee

The key names the interface mode the read was made for, but getWebViewMenu takes
no mode: the menu data service filters from its own currentMode, which starts at
'power' and is set from the same setting on its own schedule. A read that
overtakes the provider's view of a mode change is filed under the mode it asked
for while holding the other mode's items, and the cache is never invalidated. No
shipped tab item is mode-specific, so nothing differs today - the comment now
says what the first mode-specific item will need.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
… event

The cap moved from per event to per frame with the coalescing, but the only test
for it sends one outsized delta, which is far enough past the cap to fail either
way. Moving the cap back to the per-event position it held before left that test
green while thirty one-tick notches in one frame asked the parent for thirty
steps.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
… the other covers

The cover shown while the active project switches painted over the panes but took nothing away from a web view's iframe, so the keyboard stayed inside a pane nobody could see. A live check confirmed it: with the cover up, the top document still reported the editor's iframe as its activeElement, and a content-zoom chord typed there zoomed the hidden pane and persisted the level, which outlives the cover. The connection-lost and first-run covers are Radix modal dialogs and were never reachable this way.

Make this cover the same primitive: shadcn Dialog/DialogContent with the card overridden into the cover itself, the same insets, ground and z-index below modals, the built-in backdrop neutralized as the connection-lost scrim neutralizes it, and Escape/interact-outside prevented. The spinner and message stay in a role="status" live region inside the dialog, with a visually-hidden DialogTitle carrying the message as the dialog's accessible name.

DialogContent's modal path answers close-autofocus by focusing the dialog's trigger, and this cover has none, so it captures the element that had focus when it opened and refocuses it itself.

Three tests: focus leaves the web view, focus comes back when the switch ends, and the app behind the cover is aria-hidden (the containment proxy the connection-lost suite uses, since jsdom cannot drive real Tab containment). Two pre-existing tests are retargeted at the element that now carries the toolbar inset, and the localized-text assertion is scoped to the live region because the message now appears twice.

Also updates adr-web-view-content-zoom-in-iframe-shortcuts: with all three covers trapping focus, the in-view bootstrap needs no gate of its own.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
The middle-click DOM contract forces power mode through useIsPowerMode; the tab title asks useInterfaceMode instead, so the unmocked hook reached the real useSetting and every case in the file failed to render.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
…t acts

The in-view bootstrap's keyboard chord acted immediately while the wheel
tick path banked its notches for the next animation frame, so a chord
pressed inside the <=16 ms before that frame fired reached the parent
first. Ctrl+0 then wrote the default and the banked notches applied on
top of it, leaving the pane zoomed after an explicit reset; at a range
bound Ctrl+= / Ctrl+- was absorbed by the clamp, which is not
commutative, and the pane ended a step away from where the same two
actions land in arrival order. The pinch and line-mode branches already
hand pending steps over first, and the coalescer's docblock already
claimed a chord did too.

Flush the pending steps before the chord's act(), mirroring those two
branches.

Also: parenthesise the reversal test in requestZoomSteps, make act()'s
'a count is at least one' contract explicit, and record in the test file
that its frame helpers await jsdom's real requestAnimationFrame.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NJXXgHKaDjzj3KGYyV2H6G
The View menu's `role: 'viewMenu'` makes AppKit insert its own "Toggle Full Screen"
item; a hand-added `{ role: 'togglefullscreen' }` in the explicit submenu duplicated
it. Drop the hand-added item and let AppKit supply the one it inserts anyway. The
separator above it stays, so the app's own items and the system-supplied one remain
visually divided.

Found during PT-4726's macOS verification pass and filed as PT-4737; this closes it.
A unit test can't see the duplicate itself, since it is produced by AppKit rather than
by this data, but a new test guards against reintroducing the hand-added item that
caused it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RSXxZ8bg1P6f5hKWos3Ds7
When a setting's data provider has not handed back a writer yet (Input,
Switch, and UiLanguageSelector can all render before their subscription
settles), a validated change used to surface as an untranslated English
sentence wrapped around the raw setting key ("Error changing setting
platform.language: no writer is available for it yet") in the "View
error" detail. Every other rejection the component raises itself already
used a localized key.

The no-writer case now throws a dedicated SettingWriterUnavailableError
so its catch branch can show %settings_errorMessages_notWritableYet%
instead, while still logging the setting key via the existing logger.
The English wrapper sentence is now reserved for genuinely unexpected
write failures (e.g. a rejected Send/Receive write-gate).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TdDfiggLePYPTGCuNoYNCE
…located data file

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TdDfiggLePYPTGCuNoYNCE
…d papi.d.ts after rebasing onto main

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TdDfiggLePYPTGCuNoYNCE
…screen item

AppKit supplies "Toggle Full Screen" for a menu carrying role: 'viewMenu', so the platform's own
View items end at the separator before it; the combine test's expected order now matches.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TdDfiggLePYPTGCuNoYNCE
…cover closes

The workspace-updating cover restored focus to whatever element was captured
before it appeared, as long as that element was still connected. That is
right for a pane the user was working in, but wrong for a window-chrome
control: picking a project from the toolbar's "More projects…" search dialog
returns focus to the dialog's trigger (the toolbar's project-selector
button) before the cover ever captures, and that button stays connected
after the switch — so the cover handed focus back to the toolbar instead of
the new editor, leaving the first keystroke go nowhere useful. Picking a
project from the toolbar's recent list did not hit this, because whatever
the cover captured there is gone by the time the switch ends.

The close handler now only restores the captured element when it is inside
the dock's tab-content area (a pane's own DOM, including a web view's
iframe) and still connected. Otherwise — a chrome control, or an element a
tab replacement disconnected — it focuses the active editor's web view
instead, so the keyboard always lands somewhere useful once the switch ends.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TdDfiggLePYPTGCuNoYNCE
… main

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fho5RTbBzKyjhx1Gk1qU8G
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants